security: hide API credentials from process argument list#394
security: hide API credentials from process argument list#394pjt222 wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughThese changes address a security vulnerability by preventing NVIDIA API key exposure in process listings. Instead of passing the credential as a command-line argument, the key is written to temporary files with restrictive permissions (mode 0600) or passed via environment variable preservation across three entry points: bin/lib/onboard.js, bin/nemoclaw.js, and scripts/setup.sh. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/setup.sh (1)
100-112: Good credential protection pattern with a minor improvement opportunity.The implementation correctly avoids exposing the API key in process arguments:
- Uses
mktempfor unique temp fileprintf '%s'avoids trailing newlinetrap EXITensures cleanup on all exit paths$(cat "$_CRED_FILE")reads credential during shell expansionOne minor hardening opportunity: on older systems where
mktempdoesn't default to mode 600, there's a brief TOCTOU window between file creation andchmod 600. Using a subshell with restrictive umask eliminates this:🔒 Optional: atomic restrictive permissions
-_CRED_FILE=$(mktemp /tmp/nemoclaw-cred-XXXXXX) -chmod 600 "$_CRED_FILE" -printf '%s' "$NVIDIA_API_KEY" > "$_CRED_FILE" +_CRED_FILE=$(umask 077 && mktemp /tmp/nemoclaw-cred-XXXXXX) +printf '%s' "$NVIDIA_API_KEY" > "$_CRED_FILE"This ensures the file is created with 0600 permissions atomically, matching the Node.js implementation's
{ flag: "wx", mode: 0o600 }pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` around lines 100 - 112, The temp credentials file creation has a brief TOCTOU where chmod 600 runs after mktemp; change the creation to be atomic by creating the file under a restrictive umask so it is created with 0600 immediately (instead of calling chmod afterwards). Specifically, update the _CRED_FILE creation sequence (the mktemp/ chmod/ printf block referencing _CRED_FILE and used by upsert_provider) to run mktemp inside a subshell or command context that sets umask 077 before calling mktemp so the file is created with correct permissions atomically, then continue using printf, trap, and cat as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/setup.sh`:
- Around line 100-112: The temp credentials file creation has a brief TOCTOU
where chmod 600 runs after mktemp; change the creation to be atomic by creating
the file under a restrictive umask so it is created with 0600 immediately
(instead of calling chmod afterwards). Specifically, update the _CRED_FILE
creation sequence (the mktemp/ chmod/ printf block referencing _CRED_FILE and
used by upsert_provider) to run mktemp inside a subshell or command context that
sets umask 077 before calling mktemp so the file is created with correct
permissions atomically, then continue using printf, trap, and cat as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4cd2c526-e05c-49ec-a52b-93c98dd484b8
📒 Files selected for processing (4)
bin/lib/onboard.jsbin/nemoclaw.jsnemoclaw/src/commands/onboard.tsscripts/setup.sh
wscurran
added
enhancement: feature
During openshell provider create, the NVIDIA API key was interpolated directly into shell command strings, making it visible via ps aux, /proc/PID/cmdline, and process monitoring tools on shared systems. Fixes: - bin/lib/onboard.js: Write credential to temp file (mode 0600, exclusive create) and use $(cat ...) substitution so the key only exists in bash's memory during expansion, not in the parent's command string. File is cleaned up in a finally block. - scripts/setup.sh: Same temp file + cat pattern with trap EXIT cleanup. - bin/nemoclaw.js: Use sudo --preserve-env instead of interpolating the key into the command string. - nemoclaw/src/commands/onboard.ts: Added comment noting the remaining /proc exposure requires openshell --credential-file or --credential-from-env support (see PR NVIDIA#191). The credential still appears briefly in the openshell child process arguments. Full mitigation requires openshell to support reading credentials from a file or environment variable rather than CLI args. Fixes NVIDIA#325 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pjt222
force-pushed
the
security/credential-process-hiding
branch
from
6015350 to
bc814c4
Compare
|
Rebased onto current main (04012f7). Resolved conflicts with the restructured imports (credentials module, local-inference, inference-config, platform modules) and dropped the The core fix remains: API credentials are written to temp files (mode 0600, exclusive create) and read via Changes:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 811-813: The temporary credential file is created with
fs.writeFileSync(credPath, ...) before the try/finally, so if an error occurs
during or after the write the cleanup in the finally block may not run; move the
fs.writeFileSync call into the try block that wraps the operation using credPath
(the try/finally surrounding the code that deletes the file) so that the finally
always executes and removes credPath, ensuring the temporary file is always
cleaned up; update the other similar spots around the code that use credPath
(the second occurrence noted at lines ~820-822) to follow the same pattern.
- Around line 816-817: The command currently injects credentials into the child
process argv via the string `--credential "NVIDIA_API_KEY=$(cat
${shellQuote(credPath)})"` in bin/lib/onboard.js; instead, read the credential
file contents in Node, place the secret into the child process environment
(e.g., set env.NVIDIA_API_KEY = fs.readFileSync(credPath, 'utf8')) when spawning
OpenShell, and remove the sensitive `--credential` argv so the secret is not
visible in process arguments; refer to the existing shellQuote usage and the
command template and adjust the spawn/exec options to pass the env rather than
interpolating the credential into the command string.
In `@scripts/setup.sh`:
- Around line 134-137: Move the trap registration to immediately after creating
the temporary filename and before any operations that can fail: register trap
'rm -f "$_CRED_FILE"' EXIT right after assigning _CRED_FILE (the mktemp step) so
the cleanup is guaranteed even if chmod, printf, or later commands fail under
set -e; ensure _CRED_FILE is referenced in the trap and only write credentials
(printf) after the trap is in place.
- Line 143: The current command substitutes the secret into the argv via
"NVIDIA_API_KEY=$(cat "$_CRED_FILE")", which exposes it to ps/proc; instead,
check whether the openshell CLI supports passing credentials via environment
variables, stdin, or a credential-file option and use that mechanism;
specifically, inspect the openshell provider create help for flags like
--credential-file or reading from stdin, and if available change the invocation
in scripts/setup.sh to supply the path (or pipe the file to stdin) or export
NVIDIA_API_KEY into the environment before running openshell (and immediately
unset it afterward) rather than embedding the literal key in the command line.
Ensure you remove the command-substitution usage and only load the secret into a
non-argv context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3772a263-9f00-400b-9908-96e5c60be6d8
📒 Files selected for processing (3)
bin/lib/onboard.jsbin/nemoclaw.jsscripts/setup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/nemoclaw.js
| const credPath = path.join(os.tmpdir(), `nemoclaw-cred-${crypto.randomBytes(8).toString("hex")}`); | ||
| fs.writeFileSync(credPath, process.env.NVIDIA_API_KEY, { mode: 0o600, flag: "wx" }); | ||
| try { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies write occurs inside try block in bin/lib/onboard.js
awk '
/const credPath =/ {cp=NR}
/fs\.writeFileSync\(credPath/ {wf=NR}
/try \{/ && !tr {tr=NR}
END {
printf "credPath=%s write=%s try=%s\n", cp, wf, tr
exit !(tr && wf && wf > tr)
}' bin/lib/onboard.jsRepository: NVIDIA/NemoClaw
Length of output: 89
🏁 Script executed:
sed -n '809,825p' bin/lib/onboard.jsRepository: NVIDIA/NemoClaw
Length of output: 918
Move file write inside try/finally to guarantee cleanup.
If fs.writeFileSync() throws after the file is created (e.g., due to disk space or permission issues during write), the file will be left behind because the finally block is never reached if the try block is never entered.
Suggested fix
const credPath = path.join(os.tmpdir(), `nemoclaw-cred-${crypto.randomBytes(8).toString("hex")}`);
-fs.writeFileSync(credPath, process.env.NVIDIA_API_KEY, { mode: 0o600, flag: "wx" });
try {
+ fs.writeFileSync(credPath, process.env.NVIDIA_API_KEY, { mode: 0o600, flag: "wx" });
run(
`openshell provider create --name nvidia-nim --type openai ` +
`--credential "NVIDIA_API_KEY=$(cat ${shellQuote(credPath)})" ` +
`--config "OPENAI_BASE_URL=https://integrate.api.nvidia.com/v1" 2>&1 || true`,
{ ignoreError: true }
);
} finally {
try { fs.unlinkSync(credPath); } catch {}
}Also applies to: 820-822
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 811 - 813, The temporary credential file is
created with fs.writeFileSync(credPath, ...) before the try/finally, so if an
error occurs during or after the write the cleanup in the finally block may not
run; move the fs.writeFileSync call into the try block that wraps the operation
using credPath (the try/finally surrounding the code that deletes the file) so
that the finally always executes and removes credPath, ensuring the temporary
file is always cleaned up; update the other similar spots around the code that
use credPath (the second occurrence noted at lines ~820-822) to follow the same
pattern.
| `--credential "NVIDIA_API_KEY=$(cat ${shellQuote(credPath)})" ` + | ||
| `--config "OPENAI_BASE_URL=https://integrate.api.nvidia.com/v1" 2>&1 || true`, |
There was a problem hiding this comment.
Credential still reaches openshell as expanded argv content.
Using $(cat <file>) avoids inline interpolation in source strings, but the resulting credential is still passed as a command-line argument to the child process.
What is the recommended secure way in OpenShell to create/update providers without placing credential values in process arguments?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 816 - 817, The command currently injects
credentials into the child process argv via the string `--credential
"NVIDIA_API_KEY=$(cat ${shellQuote(credPath)})"` in bin/lib/onboard.js; instead,
read the credential file contents in Node, place the secret into the child
process environment (e.g., set env.NVIDIA_API_KEY = fs.readFileSync(credPath,
'utf8')) when spawning OpenShell, and remove the sensitive `--credential` argv
so the secret is not visible in process arguments; refer to the existing
shellQuote usage and the command template and adjust the spawn/exec options to
pass the env rather than interpolating the credential into the command string.
| _CRED_FILE=$(mktemp /tmp/nemoclaw-cred-XXXXXX) | ||
| chmod 600 "$_CRED_FILE" | ||
| printf '%s' "$NVIDIA_API_KEY" > "$_CRED_FILE" | ||
| trap 'rm -f "$_CRED_FILE"' EXIT |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies trap is declared before credential write in scripts/setup.sh
trap_line=$(awk '/trap .*_CRED_FILE/ {print NR; exit}' scripts/setup.sh)
write_line=$(awk '/printf .*NVIDIA_API_KEY/ {print NR; exit}' scripts/setup.sh)
echo "trap_line=$trap_line write_line=$write_line"
test -n "$trap_line" -a -n "$write_line" -a "$trap_line" -lt "$write_line"Repository: NVIDIA/NemoClaw
Length of output: 87
🏁 Script executed:
cat -n scripts/setup.sh | sed -n '130,145p'Repository: NVIDIA/NemoClaw
Length of output: 674
🏁 Script executed:
head -20 scripts/setup.sh | grep -E "^[[:space:]]*(set|#)" | head -5Repository: NVIDIA/NemoClaw
Length of output: 279
🏁 Script executed:
head -30 scripts/setup.shRepository: NVIDIA/NemoClaw
Length of output: 1047
Install the EXIT trap before any credential write.
With set -e enabled, a failure during mktemp, chmod, or printf exits the script immediately before the trap is registered, leaving the credential file on disk undeleted.
Suggested fix
_CRED_FILE=$(mktemp /tmp/nemoclaw-cred-XXXXXX)
+trap 'rm -f "${_CRED_FILE:-}"' EXIT
chmod 600 "$_CRED_FILE"
printf '%s' "$NVIDIA_API_KEY" > "$_CRED_FILE"
-trap 'rm -f "$_CRED_FILE"' EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _CRED_FILE=$(mktemp /tmp/nemoclaw-cred-XXXXXX) | |
| chmod 600 "$_CRED_FILE" | |
| printf '%s' "$NVIDIA_API_KEY" > "$_CRED_FILE" | |
| trap 'rm -f "$_CRED_FILE"' EXIT | |
| _CRED_FILE=$(mktemp /tmp/nemoclaw-cred-XXXXXX) | |
| trap 'rm -f "${_CRED_FILE:-}"' EXIT | |
| chmod 600 "$_CRED_FILE" | |
| printf '%s' "$NVIDIA_API_KEY" > "$_CRED_FILE" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup.sh` around lines 134 - 137, Move the trap registration to
immediately after creating the temporary filename and before any operations that
can fail: register trap 'rm -f "$_CRED_FILE"' EXIT right after assigning
_CRED_FILE (the mktemp step) so the cleanup is guaranteed even if chmod, printf,
or later commands fail under set -e; ensure _CRED_FILE is referenced in the trap
and only write credentials (printf) after the trap is in place.
| "nvidia-nim" \ | ||
| "openai" \ | ||
| "NVIDIA_API_KEY=$NVIDIA_API_KEY" \ | ||
| "NVIDIA_API_KEY=$(cat "$_CRED_FILE")" \ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does openshell provider createsupport reading credentials from environment variables or a credential file (instead of--credential KEY=VALUE argv), and if so what exact flags/options are available?
💡 Result:
I couldn’t find any authoritative documentation or source code for a CLI command named openshell provider create (with a --credential KEY=VALUE option) in public web search results, so I can’t verify whether it supports environment-variable credentials or a credential file, nor list the exact flags/options for doing so. The “OpenShell” results that are indexed appear to refer to unrelated projects (e.g., the Windows Start Menu app), not this CLI. [1]
If you paste the output of:
openshell provider create --help
# and ideally also:
openshell --version
openshell provider --help(or share a link to the repo/docs for this openshell), I can tell you exactly what’s supported and what the relevant flags are.
Source
- Public search results for
"openshell provider create"return unrelated “OpenShell” projects and no docs for this CLI subcommand. [1]
🏁 Script executed:
# First, let's examine the setup.sh file around line 143 to see the full context
wc -l scripts/setup.shRepository: NVIDIA/NemoClaw
Length of output: 79
🏁 Script executed:
# Read the file around line 143 to understand the context
sed -n '130,160p' scripts/setup.shRepository: NVIDIA/NemoClaw
Length of output: 1174
🏁 Script executed:
# Search for openshell definition and usage in the codebase
rg -i "openshell" --type-list | head -20Repository: NVIDIA/NemoClaw
Length of output: 448
🏁 Script executed:
# Look for openshell in the repository
fd openshellRepository: NVIDIA/NemoClaw
Length of output: 116
🏁 Script executed:
# Search for how openshell is invoked or defined
rg "openshell" -A 5 -B 5Repository: NVIDIA/NemoClaw
Length of output: 50374
🏁 Script executed:
# Find the upsert_provider function definition
rg "upsert_provider\s*\(" -A 10 scripts/setup.sh | head -50Repository: NVIDIA/NemoClaw
Length of output: 390
🏁 Script executed:
# Search for how openshell provider is invoked
rg "openshell provider" --type sh -A 2 -B 2Repository: NVIDIA/NemoClaw
Length of output: 688
🏁 Script executed:
# Look for openshell help or documentation within the repo
fd -e md -e txt | xargs grep -l "openshell provider" 2>/dev/null || echo "No docs found"Repository: NVIDIA/NemoClaw
Length of output: 72
🏁 Script executed:
# Check if there's any help text or documentation in install-openshell.sh
cat scripts/install-openshell.sh | head -100Repository: NVIDIA/NemoClaw
Length of output: 2357
🏁 Script executed:
# Let's check if there's any reference to openshell --help or version info
rg "openshell.*--help|openshell.*--version" scripts/Repository: NVIDIA/NemoClaw
Length of output: 626
🏁 Script executed:
# Check if there's any environment variable usage pattern for credentials in the codebase
rg "CREDENTIAL|credential" scripts/ -i -B 2 -A 2 | head -80Repository: NVIDIA/NemoClaw
Length of output: 954
$(cat "$_CRED_FILE") still materializes the secret in child process argv.
The temp file approach protects the parent shell, but the expanded credential value is passed to openshell provider create --credential "NVIDIA_API_KEY=<key>" where it becomes visible in the spawned process's command line (accessible via /proc/[pid]/cmdline and ps).
Investigate whether openshell provider create supports reading credentials from environment variables, stdin, or credential file paths instead of the --credential KEY=VALUE argv parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup.sh` at line 143, The current command substitutes the secret
into the argv via "NVIDIA_API_KEY=$(cat "$_CRED_FILE")", which exposes it to
ps/proc; instead, check whether the openshell CLI supports passing credentials
via environment variables, stdin, or a credential-file option and use that
mechanism; specifically, inspect the openshell provider create help for flags
like --credential-file or reading from stdin, and if available change the
invocation in scripts/setup.sh to supply the path (or pipe the file to stdin) or
export NVIDIA_API_KEY into the environment before running openshell (and
immediately unset it afterward) rather than embedding the literal key in the
command line. Ensure you remove the command-substitution usage and only load the
secret into a non-argv context.
|
Superseded by #330 which covers the same fix (passing env var name to --credential instead of KEY=VALUE). |
3 participants
Summary
During
openshell provider create, the NVIDIA API key is interpolated directly into shell command strings, making it visible to any user on the machine viaps aux,/proc/PID/cmdline, and process monitoring tools. On shared systems (DGX Spark, multi-tenant VMs), this is a real credential exposure risk.Three fixes across three code paths:
bin/lib/onboard.js$(cat ...)substitutionscripts/setup.sh$(cat ...)withtrap EXITcleanupbin/nemoclaw.jssudo --preserve-env=NVIDIA_API_KEYThe
$(cat ...)pattern ensures the key only exists in bash's memory during command substitution — it does NOT appear in the parent process's command string (the primary attack vector).Limitation: The credential still appears briefly in the
openshellchild process's/proc/PID/cmdline. Full mitigation requiresopenshellto support--credential-fileor--credential-from-env. See PR #191 for the env var approach (complementary to this PR).TypeScript CLI path (
nemoclaw/src/commands/onboard.ts): Added comment noting the remaining exposure. This path already usesexecFileSync(no shell), so it's medium severity. PR #191 addresses this path.Fixes #325
Related: #191 (env var approach for TypeScript CLI)
Test plan
nemoclaw onboardwith nvidia-nim provider — verify provider is created successfullyps aux | grep openshell— API key should NOT appear in the parent process commandscripts/setup.sh— verify same credential hiding behaviornemoclaw setup-spark— verifysudo --preserve-envworks🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Security Improvements